-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
APT-576 adding functionality to check if all orgs are the same in foreman.toml #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start so far! There's a crucial detail that I might not have explained well enough that I want to integrate here: we should compare the org in the tools against the org that the current action run is occurring within, not just against each other.
Co-authored-by: Paul Doyle <[email protected]>
src/configFile.ts
Outdated
} | ||
const manifestContent = parse(data); | ||
const sameGithubOrgSource = checkSameOrgToolSpecs(manifestContent, org); | ||
if (sameGithubOrgSource == false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (sameGithubOrgSource == false) { | |
if (!sameGithubOrgSource) { |
general practice for all languages (especially C/C++) is to never compare bools on equality to bool constants
in JS null
, undefined
and false
are interchangeable in different functions.
It is strongly recommended to use if (var)
and if (!var)
to make code change proof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need to add linter...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repo uses eslint
Anton and I synced offline and it seems like explicit null checks are recommended https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks for making the improvements!
We want to be able to enforce the same tool source orgs at the setup-foreman step of CI.
This behavior should be the default, but can be bypassed with the
allow-external-github-orgs
option